Fix/contract message signatures #2526
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Uxio0
added a commit
that referenced
this pull request
Jun 23, 2025
* Fix 1271 contract signatures * Closes #2535 and #2526 * Add migration to delete contract signatures * Add docs --------- Co-authored-by: Uxio Fuentefria <[email protected]>
Member
|
Implemented in #2539 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update 2025-05-22
After lengthy internal discussion, it was determined that the current behaviour is correct for
SafeandCompatibilityFallbackHandlerv1.3.0, while the proposed changes here are required for Safe v1.4.1+.Additionally, note that the
CompatibilityFallbackHandlerv1.3.0 will not work with Safe v1.4.1 for contract owner signatures, because thedataHash == keccak256(data)invariant is not being respected.What was wrong? 👾
It looks like the Safe transaction service is not correctly validating contract signatures for Safe messages. It uses the wrong preimage value for simulating the contract
isValidSignaturecall which causes invalid signatures to be accepted, but valid signatures to be rejected.As proof, you can "trick" the transaction service by uploading a signature for
keccak256(keccak256(SafeMessage.message)). For example, the following signature was accepted by the safe transaction service despite not being valid:https://app.safe.global/transactions/msg?safe=sep:0x2eB2E943a7877fcbC404351D7c8Ca5853B1a93D6&messageHash=0xe7ad065df60ded50e5c36a69333c2364f7592feeabfbba9ad0a17c74946d02ae
Here is a reverting simulation when trying to use the signature with
isValidSignature:https://www.tdly.co/shared/simulation/c58211dd-8e6a-40af-a935-5cb97da878a3
How was it fixed? 🎯
Using the correct pre-image for the Safe contract signatures. Depends on safe-global/safe-eth-py#1655.
Notes
I created this as a draft PR, but it is more like an issue 😅. I just thought it would be easier to point out where the exact problem is.